-
Notifications
You must be signed in to change notification settings - Fork 88
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for macOS 11.0 as a target #528
Conversation
Thanks. I guess you are adding this for a concrete use case? Most projects have migrated to cibuildwheel instead, I wonder who is still using multibuild. It would be nice to test this. I see we added a matrix for testing under different OSes in #456, but never followed up to enable the commented-out OSes. Could you try enabling that to see if this passes tests? |
The backstory here is that I'm the author of PEP 730 adding iOS support to CPython. Those changes are part of CPython 3.13, so now I'm working on the changes needed to support iOS cross-platform builds for the rest of the CPython ecosystem. I've got a work-in-progress branch for cibuildwheel; that's enough to get a really simple package building for iOS; I'm currently working on compiling Pillow for iOS as a more complex example with downstream binary dependencies, and Pillow uses multibuild to provide macOS binaries. There's a lot of similarities between the iOS and a macOS build processes; in the process of working out how Pillow's build system worked, I hit this issue, and the fix seemed obvious, so I figured I'd submit a PR. I don't have a pressing use for this specific change myself - the Pillow usage will always be via cibuildwheel, which (indirectly) sets PLAT. However, since I was in the area, I figured I'd submit the change. As for enabling tests... I'd be happy to ... except that the test matrix doesn't seem to actually test anything, and it's not obvious to me how the test suite is meant to be run. The best I can find is the travis test script, which invokes |
Just my two cents - cibuildwheel doesn't have the ability to build dependencies like library_builders.sh, so at the moment, Pillow is still using that. |
I guess the addtional CI and testing should be done in a separate PR. I am happy to put this in as-is now if it solves the macos/IOS problem, which it does, right? |
See #530. I think the macos matrix entry needs this PR to succeed, since it is running on arm64? |
a86f663
to
42943d7
Compare
Rebased on top of #530 to get CI testing |
Any idea why |
The error is consistent with something is adding Digging into the code - it looks like that's exactly what is happening: If I compile locally (with no CFLAGS defined) the compilation lines read:
but the log indicates compilations of:
That means As for the cause - I've pushed an update to the CI script that expands the test a little to do a separate macOS-13 and macOS-latest build - macOS-13 is the last x86_64 runner on Github. This seems to appease the openSSL build, but then breaks the webp build. I'm looking into that now. I guess the more thorough test would be to build an x86_64 binary on ARM64 (and vice versa) - but that will require a more substantial set of changes to how |
ba662c9
to
1a4a4c2
Compare
1a4a4c2
to
816305f
Compare
The issue with the WebP build is an interesting one - it appears to be code rot. The test suite was building WebP 0.5.0, which is almost 10 years old. The error is being raised as a compiler warning on Linux; but the ARM compiler is surfaces the warning as an error. Bumping WebP to a more recent version (1.4.0, released in April this year) avoids the issue, presumably because the bug has been resolved. A similar problem existed with hdf5 builds; hdf5 1.10.5 is also almost 10 years old, and was also generating compilation errors on x86_64. I've bumped those two package versions (including a tweak to the download URL for hdf5). |
There's one more test failure on macOS, caused because the existing code would only work on single digit python minor versions :-) |
Co-authored-by: Andrew Murray <[email protected]>
One more fix - the python installation tests contained an assertion that won't be assertable on any Python install since 3.8. Recent macOS Python installers are always universal2, not platform specific, so asserting that the macOS SDK version and architecture matches something reverse engineered from environment variables doesn't make much sense. |
b980775
to
b12a0b4
Compare
Thanks for all these fixes. I do wonder if any of those utilities are used in the wild anymore but who knows. Unfortunately there is no straightforward way to deprecate code. |
By searching the log for
The other two have to do with |
Yeah - I'm working through these at the moment. The testing process is going very slowly, because (a) the tests require the use of bash, which isn't the default shell on macOS any more, and (b) the full test suite requires sudo access, specific git configurations etc that are only reasonable in a CI environment. As a result, CI seems to be the only way I can run them all reliably.
I think I'm almost at the point where I've got them working... If I can't wrap it up soonish, though, I'll push a "comment out the tests" PR for the ones I can't make work. |
OK. Cool, thanks for all this work. It probably exceeds your budgeted time by an order of magnitude or two, for that I am grateful. I see It is also used in So my practicl question is what of all that does Pillow use in its CI? |
No problems. Consider it a downpayment on the review that I'll be asking for in a bit to add iOS support :-)
Honestly, I've been pretty much ignoring the Travis config stuff, on the basis that travis is effectively dead as an OSS concern.
I agree it's anachronistic; I'll leave the decision on whether to purge that code up to someone else. I think I've almost got it working (or, at least, passing the test suite) at this point.
Pillow uses multibuild as a way to compile the binary libraries it depends on - libpng, libjpeg-turbo, and the like. Many of these have existing recipes (hence the giflib patch that I submitted); but the ones that don't fit into the same broad configure/cmake type build structure that the tools provided by multibuild are helpful. |
Finally! @mattip @radarhere CI is now passing on macOS x86_64, macOS ARM64, and Linux. |
Co-authored-by: Andrew Murray <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks @freakboy3742 @radarhere
The
osx_utils.sh
script provides for 32-bit and 64-bit intel builds, but doesn't provide any default support for ARM64 builds.This PR adds support for MB_PYTHON_OSX_VER=11.0 mapping to arm64 builds. macOS 11.0 was the first release to support ARM64, so this is the usual SDK level targeted when an ARM64 wheel is generated.